Conversation
…ickly fail for requests for eg. am1bcc)
…H release assets at all
… for nagl models" behavior
j-wags
changed the title
j-wags
left a comment
j-wags
left a comment
There was a problem hiding this comment.
Notes to self:
- Is using lru_cache on get_model harmful when stacked on local cache dir?
- Does this block toolkit or interchange PR?
…e so they stop interfering, clean up unused monkeypatching now that release assets aren't checked
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
LGTM! A few small clarifications and suggestions for improvements in tests. None blocking
| get_model( | ||
| "my_favorite_model.pt", | ||
| ) |
There was a problem hiding this comment.
Could we be extra explicit about where the file is and isn't?
- Assert that this file is not in the installed Python package (I'd be surprised if it ever was) - feel free to do this anywhere in this test, I want to make sure that the checks into the cache and/or internet actually go there and aren't broken if we accidentally ship our "favorite" little model
- Assert that this file is present in the cache location
| # Ensure that cached files can be accessed when only doi is provided | ||
| get_model( | ||
| "my_favorite_model.pt", | ||
| doi="10.5072/zenodo.278300", |
There was a problem hiding this comment.
(not blocking) I can't figure out a good way (or reason to) test this, but I was a little surprised that an incorrect (but correctly-formatted) DOI could be passed here and it might not necessarily match the contents on Zenodo
There was a problem hiding this comment.
I somewhat addressed this in a spec clarification openforcefield/standards@2b6ef83
| match="Could not find asset with name 'FOOBAR"): | ||
| get_model("FOOBAR.pt") | ||
|
|
||
| def test_error_on_bad_file_suffix(): |
There was a problem hiding this comment.
Since this behavior should not reach out to the internet, it would be nice to have this test run with and without the socket turned off. Same with test_error_on_bad_file_suffix.
Co-authored-by: Matt Thompson <matt.thompson@openforcefield.org>
…ontext of the tests
2 participants
#42 (which isn't in a release) added a feature where, when a model filename that isn't in the python package or cache is requested, openff-nagl-models queries the releases of the openff-nagl-models repo on GitHub to see if it can be fetched. Unfortunately, these release asset queries have started returning 403 "rate limiting" errors, since several tests are given nonexistent filenames to test error behavior. While this problem could be resolved using cleverer caching to reduce the number of queries or by mocking metadata fetching in tests, I don't think the behavior is worth the complexity. This PR removes the fetching-from-GH-release-assets behavior while preserving the fetching-from-zenodo-doi behavior defined in the SMIRNOFF EP.